Skip to content

Client(iOS) - The demo's Tap to Pay sequence can offer two next steps at once - #24

Open
Alex Arguello (alex-arguello) wants to merge 57 commits into
mainfrom
alexarguello/pla-2405-clientios-the-demos-tap-to-pay-sequence-can-offer-two-next
Open

Client(iOS) - The demo's Tap to Pay sequence can offer two next steps at once#24
Alex Arguello (alex-arguello) wants to merge 57 commits into
mainfrom
alexarguello/pla-2405-clientios-the-demos-tap-to-pay-sequence-can-offer-two-next

Conversation

@alex-arguello

@alex-arguello Alex Arguello (alex-arguello) commented Aug 11, 2026

Copy link
Copy Markdown
Collaborator

Closes PLA-2405.

PaymentTapToPayQAView derived each step's status independently, and two of them read state an earlier step also read. .current and .failed are the two statuses whose content renders, so some combinations put two sets of controls, or two failures, on screen with no order between them.

tokenCheckText sessionState activationOutcome What rendered
✓ … .idle .activationFailed step 2 "do this next" and step 3 "failed"
✗ … .ready .none step 1 "failed" and step 4 "do this next"
✗ … .pendingActivation .activationFailed two failures

The file states the rule two functions above the break — "Exactly one step is ever .current" — and enableStepStatus followed it with a guard. The two below it did not.


The diff is 104 files. Three things are in it.

Read this first. 69 of those files are formatter output and contain no change of any kind to what the code does. The list below says which files are which, and there is proof at the bottom.

Files What it is
1. Behaviour 35 The step sequences, their tests, five SDK logging fixes, CI and Sonar config
2. Formatting 69 swiftformat . output, byte-for-byte
3. Nothing else No API change, no control-flow change, no dependency change

1. What changed

The sample app's step sequences — this is the ticket

The derivations move out of the three QA screens into Example/PayabliDemo/Flow/ as pure functions over plain inputs. isFinished is the rule written once: a step is finished when it is .done or .notNeeded, and each step reads the step in front of it rather than re-deriving from the session.

A screen offers one control. nextAction derives which, in the same order as the steps, and every control on the screen renders only when it is that action. That covers Recovery, which is not a step and had been deciding for itself in the view.

Behaviour changes, all agreed before the work:

  • A working step keeps its controls. The SDK's form owns its typed values in a @StateObject, so hiding the row deallocated the view model and a declined card came back to an empty form.
  • A device that was activated reads "done", where one that never needed it reads "not needed".
  • The token probe's latest answer outranks a prior success. lastResult is never cleared, so one successful payment proved the backend for the life of the app. The answer has one owner, Shared/TokenProbeResults.swift, because each screen used to keep its own: a probe run on Configuration could not reach the tab that reads it, and a tab whose backend step had finished renders no content, so its own probe was gone. Between them the rule was reachable from the tests and from nowhere in the app.
  • Every recorded failure reaches the step that can act on it. A refused activation, an enable that fails after activation, and a revoked attestation each leave the session in a different state, and each is now reported by the step whose control fixes it.
  • A failed submit shows its reason on the step that failed. The reason was written into the result row's text, and a failed form blocks the result row, so a decline offered a retry with nothing explaining it. Both card-not-present screens now draw it in the form row.

Files: Example/PayabliDemo/Flow/ (3 new), FlowTests/ (3 new), Shared/StepRow.swift (renamed from QAStepRow.swift), Shared/TokenProbeResults.swift (new), the three *QAView.swift screens, ConfigurationQAView.swift, the app entry point, project.pbxproj, a new scheme and xcconfig.

Five SDK logging fixes — the only shipped code that changes

PayabliLogger.info(_:) renders its whole message .public; the two-argument info(_:private:) marks a value .private. These call sites used the first for data the logging contract names as never-log. All pre-existing, surfaced because this branch reformatted the files.

File What was reaching the log
PayabliTTP+Charge.swift cardholder name, customerNumber, customerId, company; the /MoneyIn/update request body, which is the provider's whole response
Adapters/FiservCardReader.swift the same customer fields again; the CommerceHubResponse, carrying paymentTokens.tokenData and card expiry
TTPConfigClient.swift the /config body, whose credentials block is the reader's secretKey and apiKey
AppAttestService+Requests.swift every attestation body — /activate carries the activation code — and the assertion headers
TTPTransactionClient.swift the assertion headers: X-App-Assertion, X-App-KeyId, X-Device-Id

Endpoint, status and byte count remain. [initiate] body already used the private: overload and is unchanged.

No customer value is logged at any privacy level. .private redacts a value in a shared log and still delivers it to a local stream and to a sysdiagnose, which a cardholder name may not reach either, so the charge line now names the fields the caller populated and renders every value [REDACTED] or [nil]. That is what the Android core emits for the same record: its allowlist is deny-by-default and lists no name, address or contact field.

2. What is formatting

swiftformat --lint failed on 80 of 146 files, so the gate could not be added without formatting the tree. Running it unguarded broke the build, which is why four rules are now disabled in .swiftformat:

  • hoistAwait, hoistTry move the keyword to the start of the expression. Across an async autoclosure that changes what the code means: await XCTAssertThrowsErrorAsync(try await charge(ttp)) lost its inner await and stopped compiling.
  • noForceUnwrapInTests, noForceTryInTests rewrote Decimal(string: "25.00")! as try XCTUnwrap(...). That is a different test, and it needs the case to be throws.

The reformat is one commit, 171940a, and it is reproducible: swiftformat . on the tree before it produces those 77 files byte for byte.

3. What is CI

ci.yml ran one xcodebuild and nothing else. It now runs four jobs at once — Lint, Change report, SDK tests and Sample app step sequences — and none of them is granted a secret or a write. The two that need one, posting the change report and running the analysis, live in pr-reports.yml, which this run triggers when it finishes.

  • Lintswiftlint and swiftformat --lint. No --config on swiftlint: naming a config file makes it ignore nested ones, and Tests/.swiftlint.yml is what relaxes the rules XCTest fixtures break. With --config the count goes from 29 warnings to 104 with one serious.
  • SDK tests — the SDK suite with coverage, then the conversion Sonar reads.
  • Sample app step sequences — its own scheme. The bundle has no host application: Secrets.swift is gitignored and belongs to the app target, so the app cannot compile on a clean checkout. Verified by running the bundle with Secrets.swift moved aside.
  • Change reportScripts/classify-changes.sh sorts the diff into production, test, sample-app and tooling files, and separates the production files that can change behaviour from the ones the formatter only reflowed. It uploads the table; the job that posts it is in the other workflow.

Why the tokens live in a second workflow

GitHub runs the head revision's copy of a pull_request workflow, and a same-repository pull request is granted the repository's secrets. A branch can therefore add a step to any job in ci.yml and read whatever that job holds, whether or not the job checks the branch out. workflow_run is triggered only for a workflow file that exists on the default branch, and runs that copy, so pr-reports.yml is outside the reach of the pull request it reports on.

  • Change report comment — downloads the report from the triggering run and posts it, deleting the earlier copy so there is one report and it is the newest thing on the page. Runs nothing from the branch.
  • SonarCloud — Swift coverage has no native importer, so Scripts/xccov-to-sonarqube-generic.sh converts the .xcresult bundle in SDK tests. It exits non-zero when it finds no covered files, because an empty report reaches Sonar as 0% coverage and reads like a measurement. This job does check the head revision out, because the scanner needs the source it is analysing; it runs nothing from it.

A workflow_run job has no pull request of its own, so the number and refs travel in the artifacts as pr.json, written with jq from env rather than interpolated into a script — a branch name is chosen by whoever opens the pull request. The readers parse it and refuse a ref carrying anything but the characters git needs.

This does not take effect until it merges. A workflow_run workflow does not run while it is only on a branch, so this pull request has no change-report comment and no analysis check from here on, and neither job can be exercised before merge.

The four setup steps the two test jobs share live in .github/actions/ios-toolchain, so the Xcode selection and the simulator lookup are written once and the jobs cannot end up on different runtimes.

sonar-project.properties measures Sources and Tests. The sample app, the bridge wrappers and the vendored card-reader source are not the SDK and are not in its numbers.

What the split cost and saved

before after
wall clock 8.9 min 5.4 min
runner minutes billed 8.9 7.6
the coverage conversion 117s 10s

Billed minutes fall as well, because the conversion step gave back more than the extra checkouts cost. The critical path is SDK tests at 4.5 min, of which 184s is the suite.

The two suites both compile the SDK and cannot be made to share it. One is the package's own test action; the other consumes the package as a dependency of Example/PayabliDemo/PayabliDemo.xcodeproj, which Xcode compiles with -suppress-warnings. The intermediates path is the same and the flags are not, so pointing both at one -derivedDataPath makes each invalidate the other's objects: measured, the second invocation recompiles 87 of them either way.

The conversion is 117s to 10s because SonarSource's reference script runs xccov view --file-list and then xccov view --file once per source file, and xccov view --archive --json returns every file's line table in one call. The report is unchanged — 74 files, 7221 <lineToCover> elements, the same covered flag on every line.


Evidence that the formatting changed nothing

Both trees built at the same path, Release, simulator, so #file strings cannot differ. Fingerprint is the __TEXT,__text section of every compiled object.

before the reformat (676f412) → after it (171940a)
99 objects, __TEXT,__text byte-identical, every one

Across the whole branch, keyed on each object's full path:

base (dd1b4ec) → HEAD:  7 of 99 objects differ
  AppAttestService+Requests.o   edited: logging
  FiservCardReader.o            edited: logging
  PayabliTTP+Charge.o           edited: logging
  TTPConfigClient.o             edited: logging
  TTPTransactionClient.o        edited: logging
  FiservCardReader+Errors.o     not edited — lost the NSJSONWritingOptions
                                witness thunks belonging to the deleted
                                prettyPrintJSON, which lived in the sibling
                                file of the same type
  PayabliSDKTapToPay.o          the merged module object, carrying the new log
                                strings. Byte-identical across the reformat, so
                                it tracks real code changes only

Every changed line under Sources/ is a logger call, one of its locals, or a comment. This is a simulator Release build, so it does not speak for the device slice, and it covers Sources/ only — Tests/ and Example/ changed on purpose and do not ship.

Read the fix in two commits

eef0d37 moves the derivations out of the views and changes nothing they decide, and lands the suite red — 18 of 40 tests, 214 assertions. 676f412 adds the ordering and turns it green. Check out the first to watch it fail.

Twenty-nine review rounds then found eleven more defects in the sample app's step handling, two in log statements, four in the change-report script this branch adds, and three in its CI. Each has its own commit, and its own test wherever a suite can reach it. The step model is smaller at the end than it was after round one: acceptsActivationCode and offersRecovery collapsed into nextAction.

Tests

180 combinations for Tap to Pay — 4 probe states × 9 session states × 5 activation outcomes — and 64 per card-not-present entry point, run through both. Invariants over the whole space: at most one step renders, at most one failure, everything after the first unfinished step is blocked, a charge only from a ready terminal, the activation code only where activateDevice accepts it, Re-initialize only once the token step has finished, and a recovery reason existing exactly when a recovery control is offered and matching it. 67 tests in the demo bundle, 266 in the SDK package.

CustomerFieldRedactionTests covers the charge log's customer line: all 21 fields populated with sentinels and none reaching the rendered string, a set field distinguished from an unset one, and every field still named.

Verification

  • SDK suite, demo suite, swiftlint, swiftformat --lint, and the demo app build, all green against the combined tree
  • The demo bundle passes with Secrets.swift absent, which is the CI condition
  • Driven on the simulator against api-qa: the probe advances step 1 to done and reveals step 2; enabling fails there, and step 2 reports the reason with no retry button beside Recovery's

Not verified

Tap to Pay steps 2 to 4 have never run in any automated tier. App Attest cannot produce a real assertion on a simulator, which is what step 2 fails with above. That tier is a manual checklist walked on a device.

Sonar

The gate's one failing condition is coverage of new code; reliability, security, maintainability, duplication and hotspot review all pass. The live figures are on the check itself.

Most of the lines Sonar counts as new are formatter output from 171940a, including the card-reader paths no unit test reaches without hardware. The rest are the demo view rewiring, which no unit test reaches either: sonar-project.properties measures Sources and Tests, so the demo's own 61 step tests count towards nothing here.

🤖 Generated with Claude Code

The three QA screens each derived their step statuses inline, as private
computed properties on the View. Nothing could check them without building a
screen, and none of the defects they carry is visible to a test that renders.

This moves them to Example/PayabliDemo/Flow/ as pure functions and changes
nothing about what they decide. The bodies are a transliteration: the recorded
activation outcome is still read before the sequence has reached activation,
the charge step still consults the session rather than the step in front of it,
and the PayIn result step still keys off a result arriving rather than the form
finishing.

The vocabulary is Android's, so the two platforms name these the same things:
StepStatus, FlowStep, StepRow. The QA prefix named an audience rather than a
thing, and the demo runs against sandbox and production as readily as QA.

The tests land here, red: 18 of 40 fail, 214 assertions, over 144 Tap to Pay
combinations and 64 per card-not-present entry point. The next commit makes
them pass. Check this one out to watch them fail.

They live in a new PayabliDemoFlowTests target with no host application.
Secrets.swift is gitignored and is a member of the app target's Sources phase,
so the app cannot compile on a clean checkout and cannot host anything.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A step now reads the step in front of it rather than the state underneath. Two
steps consulting the same state is how they come to disagree about which is
next, and the file already stated the rule two functions above the break:
"Exactly one step is ever .current".

isFinished is that rule, written once. A step is finished when it is done or
notNeeded; one that is working, blocked or failed does not release the next.

The three combinations that broke it, all reachable:

  ✓ probe, session .idle, activation refused    step 2 asked and step 3 failed
  ✗ probe, session .ready                       step 1 failed, step 4 offered a charge
  ✗ probe, .pendingActivation, refused          two failures, no order between them

On the card-not-present screens the same shape: lastResult is never cleared, so
one successful payment proved the backend for the life of the app, including
while the probe was reporting the endpoint down. The probe's own answer now
outranks it once it has run.

Two behaviours change with it. A working step keeps its controls, because the
SDK's form owns its typed values in a @StateObject and hiding the row discards
them — a declined card came back to an empty form. And a device that was
activated reads "done" where one that never needed it reads "not needed"; the
outcome was already recorded and never read.

40 tests green. Verified on the simulator against api-qa: the probe advances
step 1 to done and hands step 2 the form, and mid-submit the form is still on
screen with every value intact.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
swiftformat --lint failed on 80 of 146 files, so the gate could not be added
without this. Running it unguarded broke the build, which is the reason four
rules are now off:

  hoistAwait, hoistTry move the keyword to the start of the expression. Across
  an async autoclosure that changes what the code means:
  `await XCTAssertThrowsErrorAsync(try await charge(ttp))` lost its inner await
  and stopped compiling — five errors in PayabliTTPReaderSessionRecoveryTests.

  noForceUnwrapInTests, noForceTryInTests rewrote
  `Decimal(string: "25.00")!` as `try XCTUnwrap(...)`. That is a different test,
  a nil fails it rather than crashing it, and it needs the case to be throws.

With those off the pass is a no-op, checked rather than assumed: `await` 437
before and 437 after, `try` 681 and 681. The SDK suite is green.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Copilot AI balanced review requested due to automatic review settings August 11, 2026 02:49

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Centralizes demo payment-step state derivation to prevent multiple actionable steps, aligning iOS behavior with Android.

Changes:

  • Adds ordered Tap to Pay and PayIn flow models with exhaustive tests.
  • Introduces demo-flow CI testing, formatting/lint checks, coverage, and SonarCloud integration.
  • Applies repository-wide SwiftFormat cleanup.

Reviewed changes

Copilot reviewed 92 out of 96 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
Tests/PayabliSDKTestUtilsTests/PayabliSDKTestUtilsTests.swift Formats imports.
Tests/PayabliSDKTelemetryTests/TelemetryTransportTests.swift Formats telemetry tests.
Tests/PayabliSDKTelemetryTests/PayabliSDKTelemetryTests.swift Formats imports.
Tests/PayabliSDKTapToPayTests/TTPTransactionWireFormatTests.swift Formats wire-format tests.
Tests/PayabliSDKTapToPayTests/SessionManagerTests.swift Formats tests.
Tests/PayabliSDKTapToPayTests/SecureStorageTests.swift Formats conditional test code.
Tests/PayabliSDKTapToPayTests/PayabliTTPTests.swift Formats fixtures.
Tests/PayabliSDKTapToPayTests/PayabliTTPSessionInitTests.swift Removes unnecessary async declaration.
Tests/PayabliSDKTapToPayTests/PayabliTTPReaderSessionRecoveryTests.swift Formats concurrency tests.
Tests/PayabliSDKTapToPayTests/PayabliTTPObjCInteropTests.swift Formats tests.
Tests/PayabliSDKTapToPayTests/PayabliTTPEventCodeMappingTests.swift Formats assertions.
Tests/PayabliSDKTapToPayTests/PayabliTTPErrorNSErrorTests.swift Formats assertions.
Tests/PayabliSDKTapToPayTests/FiservCardReaderTests.swift Formats platform tests.
Tests/PayabliSDKTapToPayTests/AppAttestServiceTests.swift Formats attestation tests.
Tests/PayabliSDKPayInPaymentFlowTests/PaymentCaptureClientTests.swift Removes duplicate import.
Tests/PayabliSDKPayInPaymentFlowTests/PayabliPaymentCaptureTests.swift Removes duplicate import.
Tests/PayabliSDKCoreTests/TelemetryClientTests.swift Formats imports.
Tests/PayabliSDKCoreTests/RetryPolicyTests.swift Formats retry tests.
Tests/PayabliSDKCoreTests/PayabliTransportTests.swift Formats conformance test.
Tests/PayabliSDKCoreTests/PayabliSessionTests.swift Formats imports.
Tests/PayabliSDKCoreTests/PayabliServiceTests.swift Formats error patterns.
Tests/PayabliSDKCoreTests/PayabliSDKCoreTests.swift Formats imports.
Tests/PayabliSDKCoreTests/PayabliErrorCodeMappingTests.swift Formats tests.
Tests/PayabliSDKCoreTests/PayabliEnvironmentTests.swift Formats conditional code.
Tests/PayabliSDKCoreTests/PayabliAuthTests.swift Removes unnecessary throwing declaration.
Tests/PayabliSDKCoreTests/EventMulticasterTests.swift Expands loop bodies.
Tests/PayabliSDKCoreTests/AuthenticatedTransportTests.swift Formats test helper.
Sources/PayabliSDKTestUtils/StubURLProtocol.swift Formats protocol methods.
Sources/PayabliSDKTestUtils/MockTapToPayProvider.swift Formats mock result handling.
Sources/PayabliSDKTestUtils/MockDeviceAttestationService.swift Formats mock result handling.
Sources/PayabliSDKTestUtils/MockAppAttestor.swift Expands error branches.
Sources/PayabliSDKTestUtils/InMemorySecureStorage.swift Formats lock handling.
Sources/PayabliSDKTapToPay/TTPTransactionWireFormat.swift Formats wire models.
Sources/PayabliSDKTapToPay/TTPTransactionClient.swift Formats logging and switches.
Sources/PayabliSDKTapToPay/TTPConfigWireFormat.swift Formats section comments.
Sources/PayabliSDKTapToPay/TTPConfigClient.swift Formats response decoding.
Sources/PayabliSDKTapToPay/SessionManager.swift Formats state manager declarations.
Sources/PayabliSDKTapToPay/ReaderFailureClassification.swift Formats conditional imports and patterns.
Sources/PayabliSDKTapToPay/PayabliTTPTransactionData+ObjC.swift Formats section comments.
Sources/PayabliSDKTapToPay/PayabliTTPTransactionData.swift Expands computed property.
Sources/PayabliSDKTapToPay/PayabliTTPEvent.swift Formats public event API.
Sources/PayabliSDKTapToPay/PayabliTTP+Initialize.swift Formats initialization flow.
Sources/PayabliSDKTapToPay/PayabliTTP+Charge.swift Formats charge flow and logging.
Sources/PayabliSDKTapToPay/PayabliTTP+Activation.swift Formats public activation extension.
Sources/PayabliSDKTapToPay/PayabliTTP.swift Formats facade initialization.
Sources/PayabliSDKTapToPay/KeychainStorage.swift Simplifies internal access declarations.
Sources/PayabliSDKTapToPay/EventMulticasterAlias.swift Simplifies internal access declaration.
Sources/PayabliSDKTapToPay/AppAttestWireFormat.swift Formats section comments.
Sources/PayabliSDKTapToPay/AppAttestService+Requests.swift Uses opaque request-body parameters.
Sources/PayabliSDKTapToPay/AppAttestService+Defaults.swift Formats platform defaults.
Sources/PayabliSDKTapToPay/AppAttestService+Attest.swift Formats assertion guard.
Sources/PayabliSDKTapToPay/AppAttestService+Activation.swift Formats public extension.
Sources/PayabliSDKTapToPay/AppAttestService.swift Simplifies internal initializer access.
Sources/PayabliSDKTapToPay/AppAttestor.swift Formats App Attest implementation.
Sources/PayabliSDKTapToPay/Adapters/FiservCardReader+Errors.swift Formats reader error mapping.
Sources/PayabliSDKTapToPay/Adapters/FiservCardReader.swift Formats reader adapter and logging.
Sources/PayabliSDKTapToPay/_ObjCBridging.swift Expands helper initializers.
Sources/PayabliSDKPayInPaymentFlow/PayabliPayInPaymentFlowFormConfiguration+Signature.swift Uses dictionary shorthand extension.
Sources/PayabliSDKCore/Telemetry/TelemetryEvent.swift Normalizes constant spacing.
Sources/PayabliSDKCore/Telemetry/TelemetryClient.swift Formats imports and property.
Sources/PayabliSDKCore/Public/PayabliEnvironment.swift Formats conditional environment code.
Sources/PayabliSDKCore/Networking/RetryPolicy.swift Formats ranges and branches.
Sources/PayabliSDKCore/Networking/ResponseEnvelope.swift Formats envelope properties.
Sources/PayabliSDKCore/Networking/PayabliService.swift Formats access and status range.
Sources/PayabliSDKCore/Networking/AuthenticatedTransport.swift Simplifies internal access declarations.
Sources/PayabliSDKCore/Models/PayabliError.swift Formats error properties and switches.
Sources/PayabliSDKCore/Concurrency/EventMulticaster.swift Expands loops and lock handling.
Sources/PayabliSDKCore/Auth/SessionTierValidator.swift Simplifies internal access declaration.
Sources/PayabliSDKCore/Auth/PayabliAuth.swift Formats comments and refresh closure.
sonar-project.properties Configures SonarCloud analysis and coverage.
Scripts/xccov-to-sonarqube-generic.sh Converts Xcode coverage to Sonar format.
Example/PayabliDemo/Theme/PayabliDemoColors.swift Normalizes color literals.
Example/PayabliDemo/TapToPay/TapToPayPreflight.swift Formats platform checks.
Example/PayabliDemo/TapToPay/PaymentTapToPayQAView.swift Uses centralized Tap to Pay steps.
Example/PayabliDemo/Shared/StepRow.swift Makes rows render derived flow steps.
Example/PayabliDemo/PayIn/PaymentMethodQAView.swift Uses centralized stored-method steps.
Example/PayabliDemo/PayIn/PaymentCaptureQAView.swift Uses centralized capture steps.
Example/PayabliDemo/PayIn/PayInSharedConfiguration.swift Formatting cleanup.
Example/PayabliDemo/PayabliDemo.xcodeproj/xcshareddata/xcschemes/PayabliDemoFlowTests.xcscheme Adds hostless flow-test scheme.
Example/PayabliDemo/PayabliDemo.xcodeproj/project.pbxproj Registers flow sources and test target.
Example/PayabliDemo/FlowTests/TapToPayStepsTests.swift Tests Tap to Pay state combinations.
Example/PayabliDemo/FlowTests/StepStatusTests.swift Tests shared status semantics.
Example/PayabliDemo/FlowTests/PayInStepsTests.swift Tests PayIn state combinations.
Example/PayabliDemo/Flow/TapToPaySteps.swift Derives ordered Tap to Pay steps.
Example/PayabliDemo/Flow/StepStatus.swift Defines shared flow status vocabulary.
Example/PayabliDemo/Flow/PayInSteps.swift Derives ordered PayIn steps.
Example/PayabliDemo/Debug/DebugPrefill.swift Formats debug prefill support.
Example/PayabliDemo/Configuration/DemoConfiguration.swift Formats environment configuration.
Example/PayabliDemo/Configuration/ConfigurationQAView.swift Formats configuration view.
Example/PayabliDemo/Config/FlowTests.xcconfig Configures hostless demo tests.
Example/PayabliDemo/App/PayabliDemoQAApp.swift Formats demo initialization.
Bridges/ReactNative/PayabliSDKModule.swift Formats React Native error mapping.
.swiftlint.yml Documents non-strict lint behavior.
.swiftformat Disables semantics-changing formatting rules.
.gitignore Ignores coverage artifacts.
.github/workflows/ci.yml Adds lint, demo tests, coverage, and SonarCloud.
Suppressed comments (1)

Sources/PayabliSDKTapToPay/Adapters/FiservCardReader.swift:227

  • CommerceHubResponse can contain paymentTokens.tokenData and card expiration fields, so pretty-printing the complete response through the public logger leaks data that the logging contract says must never be logged. The summary on the preceding line is sufficient; omit the response body entirely.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread Sources/PayabliSDKTapToPay/PayabliTTP+Charge.swift Outdated
Comment thread Example/PayabliDemo/Flow/TapToPaySteps.swift Outdated
Comment thread Sources/PayabliSDKTapToPay/Adapters/FiservCardReader.swift Outdated
Comment thread .github/workflows/ci.yml Outdated
CI ran one xcodebuild and nothing else. It now runs, in order: swiftlint and
swiftformat --lint, the SDK suite, the demo's step sequences, and SonarCloud.

swiftlint is invoked with no --config. Naming a config file makes SwiftLint
ignore nested ones, and Tests/.swiftlint.yml is what relaxes the rules XCTest
fixtures break: with --config the count goes from 29 warnings to 104 with one
serious, and the step fails. The config's own header claimed CI already ran
`swiftlint --strict`; it ran no lint at all, and --strict exits 2 on the
warnings in the tree today. The header now says what CI does.

The demo's tests need their own scheme and their own invocation: the bundle has
no host application, because Secrets.swift is gitignored and belongs to the app
target. Proved by running it with Secrets.swift moved aside.

Sonar follows sdk-android: SonarCloud, organization payabli, key
payabli_sdk-ios. Both prerequisites are already in place, checked rather than
assumed — the project is live under that key and SONAR_TOKEN has been a
repository secret since 28 July.

Swift coverage has no native importer, so
Scripts/xccov-to-sonarqube-generic.sh converts both .xcresult bundles into the
generic format — 120 files, 16,142 lines, 12,393 covered. It exits non-zero
when it finds no covered files, because an empty report reaches Sonar as 0%
coverage and reads like a measurement. Paths are repo-relative so the report
survives the hand-off between jobs.

Coverage exclusions mirror Android's reasoning: a SwiftUI view needs a
rendering pass, so the sample app's view directories are excluded and Flow/ is
not.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@alex-arguello
Alex Arguello (alex-arguello) force-pushed the alexarguello/pla-2405-clientios-the-demos-tap-to-pay-sequence-can-offer-two-next branch from b591ebe to 12ee549 Compare August 11, 2026 02:59
…records for it

`activateDevice` calls markError when it is refused, so the session reads
`.error` for a failure that belongs to activation. The enable step took that
`.error` as its own, and the guard added in the previous commit then blocked
activation — hiding the reason and the retry, which is what the code this
replaced was avoiding by reading the outcome first.

The enable step now hands the `.error` on when an activation failure is
recorded. Expiry is not activation's doing, so `.sessionExpired` still belongs
to enable whatever a stale outcome says, and a test holds that line.

Three tests, two of which fail before this: the point case, an invariant over
every `.error` combination that a recorded activation failure is answered by
the activation step, and the expiry case that stops the fix over-reaching.

The 144-combination sweep did not catch this. Its invariants asked whether two
steps could speak at once, never whether a recorded failure still had a voice.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The scanner failed the run outright:

  ERROR File Example/PayabliDemo/FlowTests/TapToPayStepsTests.swift can't be
  indexed twice. Please check that inclusion/exclusion patterns produce
  disjoint sets for main and test files

FlowTests lives inside Example/PayabliDemo, which sonar.sources names, while
sonar.tests names it too. Excluding it from sources leaves sonar.tests to claim
it. Tests/ needed no equivalent because it is not under a source root.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
PayabliLogger's single-argument overload renders the whole message
`.public`; the two-argument `info(_:private:)` is what marks a value
`.private`. Three call sites used the first for data the logging contract
names as never-log.

  PayabliTTP+Charge.swift  first name, last name, customerNumber, customerId
                           and company on the charge-start line. They move to
                           the `private:` call the same function already makes
                           for the rest of the customer's details.

  FiservCardReader.swift   the same fields again, on a second line that
                           duplicated the first. Removed rather than moved:
                           the line above it already carries the invoice
                           number, which is the part that is safe to publish.

  FiservCardReader.swift   the pretty-printed CommerceHubResponse. That body
                           carries paymentTokens.tokenData and the card's
                           expiry. Removed, with prettyPrintJSON, which had no
                           other caller. The summary line above it reports
                           elapsed time, byte count and card network, which is
                           what diagnosing a charge actually needs.

Pre-existing, and surfaced because this branch reformatted the files.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Swept for the shape the review found, and this is the worst instance of it.
`/config` returns ConfigCredentialsPayload, whose credentials block becomes
FiservCardReader.Credentials — secretKey and apiKey among them — and the whole
body went to the log through the overload that renders it `.public`.

Status and byte count replace it, as on the charge response.

Not the headers line above it, which was checked and is not the same problem:
the bearer is added by AuthenticatedTransport after this point, so those
headers hold the App Attest assertion rather than a credential.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings August 11, 2026 03:29

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 92 out of 96 changed files in this pull request and generated no new comments.

Suppressed comments (1)

Example/PayabliDemo/Flow/TapToPaySteps.swift:95

  • After an activation refusal the SDK sets sessionState to .error, so this makes activation .failed and renders “Enter activation code.” That action immediately calls activateDevice, whose .pendingActivation guard rejects .error; meanwhile the recovery section also renders “Re-initialize” for the same state. The screen therefore still offers two next actions, and one is guaranteed to fail. Make recovery the sole action until it restores .pendingActivation, or preserve .pendingActivation for retryable activation failures.
            // What is left is `.pendingActivation`, or the `.error` the step
            // before handed on because a refused activation put it there.
            return outcome == .activationFailed ? .failed : .current

`activateDevice` throws `.invalidState` unless the session is
`.pendingActivation`, and a refused activation leaves it `.error`. The
activation step rendered its code control there anyway, beside the
Re-initialize the same session state puts on screen: two next actions, one of
which throws on the first tap.

The sequence now carries `acceptsActivationCode` and `offersRecovery`, and the
screen reads both. The failure still reports its reason; what goes away is the
control that cannot run. Recovery was deciding this for itself in the view,
which is why no test could see it.

Two invariants over the 144 combinations: the code control appears only for
`.pendingActivation`, and never alongside Re-initialize.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Six comments stated a fact and then defended it against a choice nobody had
proposed: "Shape, not contents", "rather than here", "not the session", "not
from `hasResult`". The fact is the part that survives; the contrast reads as a
reply to a reviewer who is not there.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings August 11, 2026 03:53

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 92 out of 96 changed files in this pull request and generated 2 comments.

Suppressed comments (1)

Example/PayabliDemo/Flow/TapToPaySteps.swift:141

  • offersRecovery ignores the ordered token step. With an .error/.sessionExpired session and a latest .unreachable probe, step 1 renders its retry while the separate Recovery section also renders “Re-initialize”, recreating two competing next actions; reinitialization also cannot get through config while the token backend is known down. Gate recovery on the token step being finished, and cover this combination in the whole-space tests.
            offersRecovery: session == .error || session == .sessionExpired

Comment thread Sources/PayabliSDKTapToPay/TTPTransactionClient.swift
Comment thread Sources/PayabliSDKTapToPay/AppAttestService+Requests.swift
…iew read it

Recovery was a flag of its own, keyed on the session alone. With a failed probe
and a broken session the token step offered its retry while Recovery offered
Re-initialize: two next actions, and Re-initialize re-runs config, which a
backend known to be down cannot answer.

That is the third variant of one defect in three rounds, each time from adding
a boolean beside the steps instead of ordering the actions the way the steps are
ordered. `acceptsActivationCode` and `offersRecovery` are replaced by
`nextAction`, derived in the same pass and in the same order, and every control
on the screen renders only when it is that action.

A step still reports where it has got to; the two are separate because a step
reports failures it cannot retry. A refused activation shows its reason while
Re-initialize is the way forward.

Four invariants over the 144 combinations: the activation code only for
`.pendingActivation`, a charge only for `.ready`, Re-initialize only once the
token step has finished, and a failed probe holding the action on the probe
whatever the session says.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Two request bodies and three header dumps went to the log through the overload
that renders a message `.public`.

  AppAttestService+Requests  every attestation call. `/activate` carries the
                             activation code; the others carry the App Attest
                             key, the attestation and the assertion. The
                             response bodies went the same way.

  PayabliTTP+Charge          the `/MoneyIn/update` request body, which is the
                             provider's whole response — the same
                             `paymentTokens.tokenData` and card expiry removed
                             from the reader log one call earlier.

  TTPConfigClient,           the assertion headers: `X-App-Assertion`,
  TTPTransactionClient       `X-App-KeyId`, `X-Device-Id`.

Endpoint, status and byte count remain. `[initiate] body` already used the
`private:` overload and is unchanged.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings August 11, 2026 04:09

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 92 out of 96 changed files in this pull request and generated no new comments.

Suppressed comments (1)

Example/PayabliDemo/Flow/TapToPaySteps.swift:96

  • runActivate() records .enableFailed when activation succeeds but the follow-up initialize() fails, and runFetchConfigPhase() can leave that failure in .pendingActivation when /config still returns 403. This branch then marks enable as done, so the activation row asks for another code while the actual enableMessage is hidden in step 2. Preserve .enableFailed as an enable-step failure for this state and offer the enable retry from that row; add the corresponding .pendingActivation/.enableFailed ordering test.
            // Activation is a separate step, so reaching it means this one finished.
            case .pendingActivation: return .done
            // `activateDevice` calls markError when it is refused, so the session
            // reads `.error` for a failure that belongs to the step after this
            // one. Taking it here would block activation, which is where the
            // reason and the retry are rendered. Expiry is not activation's
            // doing, so a stale outcome does not move it.
            case .error: return outcome == .activationFailed ? .done : .failed

…activation

Activating writes `.enableFailed` when the code is accepted and the
`initialize()` after it is not, and it puts the reason in the enable step's
message: "Activated. Enabling the terminal failed — see step 2." When `/config`
answers 403 again the session goes back to `.pendingActivation`, which this step
read as finished. Step 2 was `.done` and hid the message it had just been told
to show, and step 3 asked for another code for a device already activated.

`.pendingActivation` now finishes the enable step only when no enable failure is
recorded, and the enable retry is offered from `.failed` as well as `.current`.

Two tests: the point case, and every combination carrying `.enableFailed` past
a finished token step.

`.enableFailed` was listed as written-and-never-read when this branch started,
and left that way on purpose. Reading it is what this needed.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings August 11, 2026 04:50

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 92 out of 96 changed files in this pull request and generated 1 comment.

Suppressed comments (1)

Sources/PayabliSDKTapToPay/Adapters/FiservCardReader.swift:222

  • The charge duration is still emitted only as a per-instance log line. Reader charge is a money-movement critical path, so this cannot answer aggregate latency or failure-rate questions; record the duration on the charge/NFC telemetry as a histogram or span, and reserve this log for diagnosing one invocation.

Comment thread Example/PayabliDemo/Flow/TapToPaySteps.swift Outdated
`testCardReadFailureKeepsTheSessionReady` failed on CI and passed on a re-run of
the same commit. The cause is not the runner: `bounded` slept a hardcoded five
seconds while `boundSeconds` sat beside it, declared, documented and never
referenced.

Its own comment says what the number is for:

    for the slowest machine that runs this, not the fastest: an existing test
    in this suite takes eighteen seconds on CI and milliseconds locally.

Five seconds is under a third of the eighteen that comment records. Measured
here, the test runs in two to nine milliseconds, so the bound was three orders
of magnitude tighter than the work and still short of what CI does.

What the bound catches is a call that never returns, and sixty seconds catches
that as surely as five while leaving room for a machine that stalls.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`Latch.enter` incremented the count and `Latch.hold` stored the continuation, as
two calls on the actor. The test waited on the count, so it could reach
`release(2)` after the second run was numbered and before it was registered.
That release found nothing to resume and was dropped, and the run then waited
for a release that had already happened: `await second.value` never returns and
the case hangs until XCTest gives up, which reports nothing about the defect.

`hold()` now numbers and registers in one call, with no suspension between the
two lines, so a run the test can count is a run the test can release.

Ten consecutive runs of the class pass. The previous structure was not observed
hanging, because the interleaving it allows is the rarer one; it is removed
rather than measured, since the failure it produces is a hang.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The job and the step both carried continue-on-error, for a fork's pull request,
whose token is read-only and cannot post. That made every outcome green: a
report that had stopped posting looked exactly like one that posted, and the
only evidence was a comment nobody noticed was stale.

The fork case is skipped by the same condition the analysis job already uses, so
the case that cannot work does not run and every other failure is a red job.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The report was edited in place, which kept it to one comment and left it
wherever it was first posted. On a review of this length that is pages above the
discussion, marked only "edited", so six pushes updated a comment that never
moved and the report read as broken.

Every earlier copy is deleted and a fresh one posted, so there is still exactly
one report and it is the newest thing on the page. The marker the script writes
is what finds them; on this pull request it matches 1 of 12 comments, checked
before the deletion was written.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings August 12, 2026 04:12
@github-actions

Copy link
Copy Markdown

Change report

dd1b4ec…fc436ec · 103 files

Review surface

Category Files
Production code (ships in the SDK) 42
Test code 28
Sample app 22
Build, CI and tooling 10
Bridge wrappers 1

Files added, deleted and renamed

17 added, 0 deleted, 1 renamed.

A	.github/actions/ios-toolchain/action.yml
A	.github/actions/lint-tools/action.yml
A	Example/PayabliDemo/Config/FlowTests.xcconfig
A	Example/PayabliDemo/Flow/PayInSteps.swift
A	Example/PayabliDemo/Flow/StepStatus.swift
A	Example/PayabliDemo/Flow/TapToPaySteps.swift
A	Example/PayabliDemo/FlowTests/PayInStepsTests.swift
A	Example/PayabliDemo/FlowTests/StepStatusTests.swift
A	Example/PayabliDemo/FlowTests/TapToPayStepsTests.swift
A	Example/PayabliDemo/FlowTests/TokenProbeResultsTests.swift
A	Example/PayabliDemo/PayabliDemo.xcodeproj/xcshareddata/xcschemes/PayabliDemoFlowTests.xcscheme
R066	Example/PayabliDemo/Shared/QAStepRow.swift	Example/PayabliDemo/Shared/StepRow.swift
A	Example/PayabliDemo/Shared/TokenProbeResults.swift
A	Scripts/classify-changes.sh
A	Scripts/print-test-failures.sh
A	Scripts/xccov-to-sonarqube-generic.sh
A	Tests/PayabliSDKTapToPayTests/CustomerFieldRedactionTests.swift
A	sonar-project.properties

A rename shown below R100 was edited as well as moved.

Production code (Sources/)

42 modified, 0 added, 0 deleted, classified by whether the change can alter behaviour.

Classification Files Reviewer action
Formatting only, semantically inert 36 None. Reproduced by running the formatter over the base revision.
Comments and documentation only 0 Read for accuracy. Compiles to the same code.
Declarations or statements changed 6 Review. This is where behaviour can change.

Files that can change behaviour

Code counts declarations and executable statements that differ once
formatting is normalised, on both sides of the diff, so an altered line
counts twice and a line that only moved still counts. Order is behaviour:
validation moved to after the network call it guards is a change made
entirely of unaltered lines. Docs is the same count for comments.

Code Docs File
65 17 Sources/PayabliSDKTapToPay/PayabliTTP+Charge.swift
19 10 Sources/PayabliSDKTapToPay/Adapters/FiservCardReader.swift
12 3 Sources/PayabliSDKTapToPay/AppAttestService+Requests.swift
9 5 Sources/PayabliSDKTapToPay/TTPTransactionClient.swift
8 3 Sources/PayabliSDKTapToPay/TTPConfigClient.swift
2 52 Sources/PayabliSDKTapToPay/PayabliTTP.swift

Public API surface

No declaration a consumer can see was added or removed, across modified,
renamed, added and deleted files under Sources/. This counts a member of a
public extension and an enum's cases, neither of which carries the keyword.
It is a text scan rather than a compiled comparison, so read it as nothing
found to look at, not as a source-compatibility guarantee.

Tests

6 production files changed behaviour, alongside 32 changed test files.

@alex-arguello

Copy link
Copy Markdown
Collaborator Author

Suppressed comments · a test that can hang, and the write token

The latch could drop a release, and the test would hang

The wait condition observes entered, which is incremented before the task separately calls hold. The test can therefore call release(2) before continuation 2 is stored; that release is lost and second.value hangs.

Fixed · 4ea808c

Correct. Latch.enter and Latch.hold were two calls on the actor, the test waited on the count between them, and a release landing in that window found nothing to resume. The run then waited for a release that had already happened, so the case hangs until XCTest gives up, reporting nothing about what it was checking.

Change hold() numbers the run and registers its continuation in one call, with no suspension between the two lines, so a run the test can count is a run the test can release.
Test The class itself, run ten consecutive times. The previous structure was not observed hanging — the interleaving it allows is the rarer one — so it is removed rather than measured, since a test that hangs is exactly what must not go in.

The comment job's write token

This job still exposes a write-scoped token to pull-request-controlled code: the run: block itself comes from the PR's version of this workflow, even though the job does not check out the branch.

Deferred · to a later commit on this pull request. Same finding as the inline comment on the analysis job, answered in that thread: the reasoning, the documentation it rests on, and why the move to workflow_run follows rather than leads.

Two commits go in first, because they are what makes the move checkable at all.

d7a8f33 removes continue-on-error from the job and the step, and skips the fork case the same way the analysis job does. Both were green whatever happened, so a report that had stopped posting was indistinguishable from one that posted.

fc436ec deletes the earlier report and posts a fresh one instead of editing in place. Editing kept it to one comment and left it wherever it was first posted, which on this review is pages above the discussion and marked only "edited": six pushes updated a comment that never moved. The marker matches 1 of 12 comments here, checked before the deletion was written.

Change For this finding, none yet.
Test None. The next push is what shows whether the job works, which is the point of doing these two first.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 99 out of 103 changed files in this pull request and generated no new comments.

@sonarqubecloud

Copy link
Copy Markdown

Quality Gate Failed Quality Gate failed

Failed conditions
68.2% Coverage on New Code (required ≥ 80%)

See analysis details on SonarQube Cloud

`ci.yml` is triggered by `pull_request`, and GitHub runs the head revision's
copy of it. A same-repository pull request is granted the repository's secrets,
so a branch could add a step to the analysis job and read `SONAR_TOKEN`, or to
the comment job and read the write token. Splitting the token into a job that
checks nothing out did not close that: the job's steps are still written in the
branch's copy of the file.

Both move to `pr-reports.yml`, triggered by this workflow finishing. GitHub
triggers `workflow_run` only for a workflow file that exists on the default
branch, and runs that copy, so those two jobs are no longer editable by the
pull request they report on. Nothing left in `ci.yml` holds a secret or a write.

A `workflow_run` job has no pull request of its own, so the number, head ref and
base ref travel in the artifacts as `pr.json`. They are passed through `env` and
written with `jq`: a branch name is chosen by whoever opens the pull request and
`${{ }}` pastes it into the shell before the shell sees it, so a branch called
`$(...)` would otherwise run as a command in a job that now holds a token. The
readers parse the file rather than sourcing it, and refuse a ref carrying
anything but the characters git needs. On a push there is no `pr.json`, and its
absence is what selects a branch analysis over pull request decoration.

Checked locally against the three inputs: a real pull request yields the three
`sonar.pullrequest` arguments, a branch named `evil$(id)` is refused and reddens
the job, and an absent file analyses a branch. `actionlint` passes on both
files.

What this cannot buy: the scanner needs the head revision in the workspace, so
that job still checks the pull request's source out while holding the token. It
runs nothing from the tree, and the coverage it reads was produced by the run
that triggered it, but the protection is over the workflow definition rather
than over what is analysed.

The cost, stated because it is not recoverable from the diff: a `workflow_run`
workflow does not run until it is on the default branch, so this pull request
has no change-report comment and no analysis check until it merges, and neither
job can be exercised before then.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings August 12, 2026 04:45

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 100 out of 104 changed files in this pull request and generated 2 comments.

Suppressed comments (2)

.github/workflows/ci.yml:27

  • The workflow does not set top-level permissions, so these PR-controlled jobs inherit the repository's default GITHUB_TOKEN grants. Because checkout persists that token and the branch controls local actions and scripts, a same-repository PR can recover any default write grant, contrary to the isolation described here. Explicitly restrict the workflow token to read-only contents and keep narrower overrides per job.
# Nothing here is granted a secret or a write. Posting the change report and
# running the analysis both need one, and both live in `pr-reports.yml`, which
# this run triggers when it finishes. That file is read from the default branch,
# so a pull request cannot edit the jobs that hold the tokens; every job below
# is read from the pull request's own revision, which is why none of them may
# hold one.

Example/PayabliDemo/Flow/PayInSteps.swift:73

  • A shared probe can still erase unsent form input. After the backend has been proven and the user has typed into the SDK form, starting the same probe from Configuration publishes .checking; with isSubmitting == false, this branch makes the backend unfinished, the form becomes .blocked, and StepRow removes the form's @StateObject. The completed probe then returns an empty form. Preserve the mounted form once the user has reached it (including while a later probe is checking or fails), or otherwise keep its state alive while the sequence reports the newer probe result.
            switch progress.tokenCheck {
            // Before the outcome, or the step offers its button over a request
            // already in flight.
            case .checking: return .inProgress

Comment thread .github/workflows/pr-reports.yml Outdated
Comment thread .github/workflows/pr-reports.yml Outdated
The previous commit moved the tokens into a workflow the branch cannot edit and
then handed that workflow a number the branch chose. `ci.yml` runs from the head
revision, so `pr.json` was attacker-controlled input: a branch could write
another open pull request's number into it and have the trusted workflow delete
that pull request's change report and post whatever this branch's script had
produced in its place, and file this revision's analysis against it.

Both jobs now read `github.event.workflow_run.pull_requests[0]`, which GitHub
fills in from the triggering run and a branch cannot write to. `pr.json` is gone
from both artifacts rather than validated, since checking the shape of a number
says nothing about whose number it is.

Both jobs also require the triggering run's head repository to be this one. A
fork has no entry in `pull_requests`, so there would be nothing trustworthy to
name, and the analysis job would otherwise check a fork's revision out while
holding the token.

The refs still reach the scanner as command arguments, so they are matched
against the characters git needs rather than trusted.

Checked locally against the four inputs the step can receive: a pull request
yields the three `sonar.pullrequest` arguments, an absent number analyses a
branch, and a branch named `evil$(id)` or a number reading `24; rm -rf /` is
refused and reddens the job. `actionlint` passes on both files.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings August 12, 2026 05:26

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 100 out of 104 changed files in this pull request and generated 2 comments.

Suppressed comments (1)

.github/workflows/pr-reports.yml:79

  • This selects comments by marker only, so a reviewer comment that quotes or copies the report marker is also deleted. Restrict deletion to comments authored by the Actions bot; the workflow should never remove user-authored review discussion.
            | jq -s 'add | map(select(.body | contains("<!-- change-report -->")) | .id) | .[]')

Comment thread .github/workflows/ci.yml
Comment thread .github/workflows/pr-reports.yml
`ci.yml` set no top-level `permissions`, so every job took the repository's
default grant. That default is read-only today, and it is a repository setting
rather than a property of this file: raising it would hand a write to jobs that
run the pull request's own local action and scripts, which is the opposite of
what the comment at the top of the file claims.

The workflow now pins `contents: read`, and each checkout sets
`persist-credentials: false`, since nothing here pushes and the token was
otherwise left in `.git/config` for whatever ran next.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The answer was overwritten with "Checking…" the moment a run started. The probe
is shared, so a run started on the Configuration tab retracted a verdict a
payment tab had already acted on: the backend step went from `.done` to
`.inProgress`, which is not finished, so the form step became `.blocked`,
`StepRow` dropped the row's content, and the `@StateObject` holding what a payer
had typed went with it. The completed probe then returned an empty form.

The previous commit covered this for a submission in flight only, by reading
`isSubmitting` before the probe. A payer part way through the form has not
submitted anything, so that branch never fired.

The last settled answer and the set of runs in flight are now separate. A run
publishes only when it finishes, so an earlier verdict stands until a new one
lands. `check(_:)` reports `.checking` only when there is no earlier answer to
keep, which is the first run of all; `display(for:)` does report the run, so a
row a person is looking at still says the button did something. The screens read
`isRunning(_:)` for the control they would otherwise offer twice.

`testARunInFlightKeepsTheAnswerTheLastOneSettledOn` settles one run, holds the
next open, and asserts the step still reads `.reachable` while the row reads
"Checking…". Restoring the overwrite fails that test and no other. 69 tests in
the demo bundle, up from 67.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The comments to remove were selected by the marker alone, so a reviewer quoting
the report in discussion would have been quoting the marker, and this job holds
a write token. Review discussion is not this workflow's to delete.

The author has to be `github-actions[bot]` as well. Checked against a list
holding a reviewer's comment that quotes the marker, another bot's, and the real
report: only the real report is selected. On this pull request it still matches
the one comment it posted.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The scanner reads `sonar-project.properties` out of the revision it is
analysing, and that file names the server and the project. A same-repository
branch could point `sonar.host.url` at a server of its own and this step would
hand it SONAR_TOKEN, or change the identity and publish the analysis into
another project it can reach. Checking the branch out without running it does
not cover a tool that takes its instructions from the tree.

The host, organization and project key are now given on the command line, which
wins over the file, from the workflow a pull request cannot edit. They duplicate
the first three lines of `sonar-project.properties` deliberately. What a branch
may still choose is what gets measured, which is the rest of that file.

Checked directly: the three properties lead the argument list on the pull
request path and on the branch path, and a ref that is not one is still refused.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings August 12, 2026 05:38
@alex-arguello

Copy link
Copy Markdown
Collaborator Author

Suppressed comments · deleting by marker, and a probe that empties a form

Deleting by marker alone

This selects comments by marker only, so a reviewer comment that quotes or copies the report marker is also deleted. Restrict deletion to comments authored by the Actions bot; the workflow should never remove user-authored review discussion.

Fixed · c5b3703

Correct, and the reachable case is ordinary rather than adversarial: quoting the report in a reply quotes its marker, and this job holds a write token.

Change The author has to be github-actions[bot] as well.
Test Run against a list holding a reviewer's comment that quotes the marker, another bot's comment carrying it, and the real report: only the real report is selected. On this pull request the selector still matches the one comment the workflow posted.

A probe from Configuration can empty a form being filled in

After the backend has been proven and the user has typed into the SDK form, starting the same probe from Configuration publishes .checking; with isSubmitting == false, this branch makes the backend unfinished, the form becomes .blocked, and StepRow removes the form's @StateObject.

Fixed · de919d8

Confirmed, and ab7e283 covered only half of it. That commit read isSubmitting before the probe, which holds the form for a submission already in flight; a payer part way through typing has submitted nothing, so the branch never fired for the case that loses the most.

The cause was the store, not the sequence: the answer was overwritten with "Checking…" the moment a run started, so a verdict a payment tab had already acted on was retracted by a run started on another tab.

Change The settled answer and the set of runs in flight are now separate, and a run publishes only when it finishes. check(_:) reports .checking only when there is no earlier answer to keep, which is the first run of all, so a later run leaves the step where it was until it lands. display(for:) does report the run, because a row someone is looking at should say the button did something. The screens read isRunning(_:) for the control they would otherwise offer twice.
Test testARunInFlightKeepsTheAnswerTheLastOneSettledOn settles one run, holds the next open, and asserts the step still reads .reachable while the row reads "Checking…". Restoring the overwrite fails that test and no other. 69 tests in the demo bundle, up from 67, and the sample app builds.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 100 out of 104 changed files in this pull request and generated no new comments.

Suppressed comments (1)

.github/workflows/pr-reports.yml:29

  • Concurrent workflow_run executions for the same pull request are not serialized. If another CI run completes while an earlier report job is still running, both jobs can delete/list before either posts and leave duplicate or stale change-report comments. Add job-level concurrency keyed by the authoritative pull-request number so a newer report cancels the older one.
  comment:
    name: Change report comment
    runs-on: ubuntu-latest

`pr-reports.yml` had no concurrency of its own. `ci.yml` cancels a superseded
run, but cancelling is not instant: a run already at its last step finishes, and
the push that superseded it produces a second, so two report jobs can be in
flight for one pull request. Both list the comments, both delete what they
found, and the loser of that race deletes a comment that is already gone and
reddens on it; on the other ordering both post and the pull request carries two.

Each job is now keyed by the pull request the event names, and a newer run takes
an older one's place, since the older one is reporting on a revision that has
been replaced.

The analysis has the same race for the same reason, and was not in the review:
two analyses of one pull request reach the server in whichever order they
finish, so the later revision's numbers can be overwritten by the earlier one's.
A push run names no pull request and is keyed by its branch.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings August 12, 2026 05:52
@alex-arguello

Copy link
Copy Markdown
Collaborator Author

Suppressed comment · pr-reports.yml:29 · two report jobs for one pull request

Concurrent workflow_run executions for the same pull request are not serialized. If another CI run completes while an earlier report job is still running, both jobs can delete/list before either posts and leave duplicate or stale change-report comments.

Fixed · 295e297

Reachable despite ci.yml cancelling superseded runs, because cancelling is not instant: a run already at its last step finishes, and the push that superseded it produces a second. A cancelled run is skipped by the condition above, but a run that beat the cancellation is not.

Both orderings are bad. Two jobs that list before either posts both delete what they found, and the second delete is of a comment that is already gone, which reddens that job under set -e; two that post leave the pull request carrying two reports.

Change Each job is keyed by the pull request the event names, and a newer run takes an older one's place, since the older one is reporting on a revision that has been replaced.

The analysis job has the same race and was not in the review: two analyses of one pull request reach the server in whichever order they finish, so the later revision's numbers can be overwritten by the earlier one's. It is keyed the same way, falling back to the branch for a push, which names no pull request.

Test None automated; a workflow_run workflow cannot run from a branch. actionlint passes, and both groups were read back from the parsed workflow rather than from the diff.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 100 out of 104 changed files in this pull request and generated no new comments.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants